Skip to content

Refactor shim interfaces to not be implementable externally #3100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2025
Merged

Refactor shim interfaces to not be implementable externally #3100

merged 1 commit into from
Jun 3, 2025

Conversation

@VenelinMartinov
Copy link
Contributor

@VenelinMartinov VenelinMartinov commented May 29, 2025 •
edited
Loading

This PR is a pure refactor. The shim interfaces are not intended to be implemented externally. They change often and due to the fact they are public we have kept accruing new variants to keep the public interface backwards compatible which add unnecessary complexity.

This PR adds InternalInterface interface with a private method which is only implemented by internalinter.Internal. This can be embedded in interfaces which are not intended to be implemented outside of the bridge. Implementers in the bridge can embed the implementation of InternalInterface but external users can not.

This means that we are free to add methods to interfaces which are not intended to be implemented externally. This will in turn allow us to delete some of the Schema and Resource variants under shim.

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented May 29, 2025 •
edited
Loading

This change is part of the following stack:

Change managed by git-spice.

@VenelinMartinov VenelinMartinov mentioned this pull request May 29, 2025
Merged
@VenelinMartinov VenelinMartinov changed the title Refactor shim interface to not be implementable externall Refactor shim interface to not be implementable externally May 29, 2025
@VenelinMartinov VenelinMartinov changed the title Refactor shim interface to not be implementable externally Refactor shim interfaces to not be implementable externally May 29, 2025
@VenelinMartinov VenelinMartinov requested review from a team and blampe May 29, 2025 11:31
@VenelinMartinov VenelinMartinov force-pushed the vvm/refactor_shim_internal branch from faf6286 to d0ef58b Compare May 29, 2025 11:33
@codecov
Copy link

codecov bot commented May 29, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 84.21053% with 12 lines in your changes missing coverage. Please review.

Project coverage is 68.79%. Comparing base (2093fe3) to head (5fa7b74).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
pkg/pf/internal/schemashim/resource_map.go 72.72% 3 Missing ⚠️
pkg/pf/proto/object.go 0.00% 2 Missing ⚠️
pkg/internal/internalinter/internalinter.go 0.00% 1 Missing ⚠️
pkg/pf/proto/block.go 85.71% 1 Missing ⚠️
pkg/pf/proto/element.go 75.00% 0 Missing and 1 partial ⚠️
pkg/tfshim/sdk-v1/instance_state.go 0.00% 1 Missing ⚠️
pkg/tfshim/sdk-v1/provider.go 75.00% 1 Missing ⚠️
pkg/tfshim/sdk-v1/resource.go 83.33% 1 Missing ⚠️
pkg/tfshim/sdk-v2/resource.go 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #3100   +/-   ##
=======================================
  Coverage   68.78%   68.79%           
=======================================
  Files         335      336    +1     
  Lines       43560    43575   +15     
=======================================
+ Hits        29962    29976   +14     
- Misses      11887    11888    +1     
  Partials     1711     1711

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This was referenced May 29, 2025
Merged
Merged
Base automatically changed from vvm/pf_dynamic_diff_err to main May 29, 2025 13:13
@VenelinMartinov VenelinMartinov force-pushed the vvm/refactor_shim_internal branch from d0ef58b to 5fa7b74 Compare May 29, 2025 13:14
@t0yv0
Copy link
Member

t0yv0 commented May 29, 2025

Why not just use comments to the purpose? If something out there manages to use these with success, although they are unstable, why be extra strict here and promote additional breakage? That said we are using downcasts aggressively for example on values of type shim.InstanceState so I doubt anything out there is actually going to be broken.

t0yv0
t0yv0 reviewed May 29, 2025
View reviewed changes
pkg/internal/internalinter/internalinter.go
@@ -0,0 +1,9 @@
package internalinter
Copy link
Member

@t0yv0 t0yv0 May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this is public? :) So a determined user could still implement it?

Copy link
Contributor Author

@VenelinMartinov VenelinMartinov May 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - this is in the internal directory, so no user outside of the bridge can use it.

Copy link
Member

@t0yv0 t0yv0 May 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay !

@VenelinMartinov
Copy link
Contributor Author

VenelinMartinov commented May 29, 2025 •
edited
Loading

I'd rather we get ahead of it and just prevent any external implementations. We are then completely free to add methods to these interfaces.

I do agree there are unlikely to be any.

@t0yv0
Copy link
Member

t0yv0 commented May 29, 2025

Since there's likely zero impact I'll approve. If we hear from users we can reconsider and back out.

@t0yv0 t0yv0 self-requested a review May 29, 2025 13:21
@VenelinMartinov VenelinMartinov merged commit 63f29f1 into main Jun 3, 2025
136 checks passed
@VenelinMartinov VenelinMartinov deleted the vvm/refactor_shim_internal branch June 3, 2025 16:30
@VenelinMartinov VenelinMartinov added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@t0yv0 t0yv0 t0yv0 approved these changes

@blampe blampe Awaiting requested review from blampe

Assignees

No one assigned

Labels

impact/no-changelog-required This issue doesn't require a CHANGELOG update

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@VenelinMartinov @t0yv0